fix: Referencer, Name conflicts, Mapped unions, etc. #1498

nemnandor · 2023-10-19T09:45:53Z

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Have you written unit tests?
Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
This PR is associated with an existing issue?

Closing issues

closes #477
closes #623
closes #681
closes #1191
closes #1204
closes #1266
closes #1447
closes #1472

If this is a new feature submission:

Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Fixes:

I have solved locally some problems which are needed for my company. After that I read the TSOA commit guides, so some fixes maybe have no related issues.

Namespaces: Every type should be stored with their full-path namespaced types. Even if the source code not uses this name. This reduces conflicts between types.
Name conflicts: If every reference type name parts are uniq, then the full type name can be handled as uniq. In this type name, we need to sign the jsdoc params if exists, because its they can make differences in the result. Somehow the reference type names are transformed with an irreversible getRefTypeName(), here we need to check if the result name comes from the same original type names, or from more different names.
Handling interfaces - namespaces - enums which have more than one declaration
Keep jsdoc comments + defaults after mapping - as intellisense shows
Do not use original definition in mapped types, because type can change.
Accept unions, intersections, nulls, enums in mapped type.
Fix referencer issues. TypeToTypeNode & TypeNodeToType functions are not exactly the inverse function of each other -> use the original types, and remove conversions.
Do not name more complicated reference type internal parts as "any". - It is needed to recognize name conflicts.
Better keyof handling
Do not mark required if a field has a default value. (Not too relevant, but more beautiful)
Default values from JSdoc uses the expected type. @default true is not a string, but a boolean.

Potential Problems With The Approach

My code sometimes returns with an equivalent type definition, not the original (If Extended interface doesn't produce an allOf type #1490 is a bug, then I made bugs). Check original test changes.
I have not implemented Partial or Partial-like mapped types. I can implement it, but not really know if it is interesting or not/where is the end of this strange types.
I solved name conflict handling, but for it I generate the type name every time (previously tsoa made it in the else branch, but normally we used the typescript name). For some name, some type formations are not implemented, like:
type A = Partial<{ [a in keyof B]: string }>
But there is a workaround:
type AInternal = { [a in keyof B]: string }; type A = Partial<AInternal>
A mapped type can change the type of a member, but can not change the jsdoc of it (sometimes deletes it, but sometimes not). So we can create a number member (with default value 2, and minimum 0), and after that, we can construct a mapped type which can change the type to string. Everything is fine with intellisense, but we can define a type which should have a value, because if not, the default value will have another type, so typecheck fails. Now TSOA handles type restrictions which are related only to one type, so in this case there will be not relevant type checks, not really interesting.
If a type (interface) has multiple declaration, and both declarations contains the same member with different jsdoc, the program chooses one of them.
Union types in typescript are a little bit strange, A | B ~ { some properties of A & some properties of B}. TSOA validation not handles them correctly (TSOA validation implementation: A | B = Only A | Only B). In a type with multiple different additionalProperties type I use type unions to "approxximate" the expected operation. It is ok until someone fixes TSOA validation. But I'm not sure, that most programmers love or hate this TSOA validation bug, I enjoy it, it is more strict than ts.

I'm sure, that there are a lot of breaking changes.

Test plan

Added test type literals to the TestModel interface. Every literal checks one solved problem. The members represents one test case.

github-actions

Hello there nemnandor 👋

Thank you and congrats 🎉 for opening your first PR on this project.✨

We will review the following PR soon! 👀

nemnandor · 2023-10-19T09:47:46Z

tests/unit/swagger/definitionsGeneration/definitions.spec.ts

@@ -1119,7 +1137,7 @@ describe('Definition generation', () => {
              {
                properties: {
                  list: {
-                    items: { $ref: '#/definitions/ThingContainerWithTitle_string_' },
+                    items: { type: 'number', format: 'double' },


Equivalent change, but maybe the previous solution was better readable

nemnandor · 2023-10-19T09:49:26Z

tests/unit/swagger/definitionsGeneration/definitions.spec.ts

@@ -1169,13 +1188,13 @@ describe('Definition generation', () => {
                    format: undefined,
                  },
                  indexedResponseObject: {
-                    $ref: '#/definitions/Record_id.any_',
+                    $ref: '#/definitions/Record_id._myProp1-string__',


Previous name generation worked well only if we were not in a nested mapped type. Fixed to check&handle name conflicts

nemnandor · 2023-10-19T09:49:59Z

tests/unit/swagger/definitionsGeneration/definitions.spec.ts

                    description: undefined,
                    example: undefined,
                    format: undefined,
                  },
                  indexedType: { type: 'string', default: undefined, description: undefined, format: undefined, example: undefined },
-                  indexedTypeToAlias: { $ref: '#/definitions/IndexedInterfaceAlias', description: undefined, format: undefined, example: undefined },
+                  indexedTypeToAlias: { $ref: '#/definitions/IndexedInterface', description: undefined, format: undefined, example: undefined },


Equivalent change, but maybe previous was better readable in some cases

nemnandor · 2023-10-19T09:51:52Z

tests/unit/swagger/definitionsGeneration/definitions.spec.ts

@@ -131,7 +131,7 @@ describe('Definition generation', () => {
          'TestSubModelContainerNamespace.InnerNamespace.TestSubModelContainer2',
          'TestSubModel2',
          'TestSubModelNamespace.TestSubModelNS',
-          'TsoaTest.TestModel73',
+          'tsoaTest.TsoaTest.TestModel73',


For safe name conflict handling, every type needs to use its fully namespaced name. Not accepted to think, that TsoaTest type is equal to another interface in the tsoaTest namespace

nemnandor · 2023-10-19T09:52:15Z

tests/unit/swagger/definitionsGeneration/definitions.spec.ts

@@ -564,6 +564,7 @@ describe('Definition generation', () => {
                  default: undefined,
                },
              },
+              required: ['record-foo', 'record-bar'],


Wrong previous tests

nemnandor · 2023-10-19T09:54:09Z

tests/unit/swagger/definitionsGeneration/definitions.spec.ts

@@ -834,7 +852,7 @@ describe('Definition generation', () => {
              example: 42,
              minimum: 42,
              maximum: 42,
-              default: '42',
+              default: 42,


Now default types are ok, so we can use default values with @default jsdoc comment

nemnandor · 2023-10-19T09:57:24Z

packages/cli/src/metadataGeneration/exceptions.ts

@@ -11,7 +11,7 @@ export class GenerateMetadataError extends Error {
 }



Small thing, do not crash while trying to make exceptions to throw (or when make warning messages).

nemnandor · 2023-10-19T09:57:54Z

packages/cli/src/metadataGeneration/metadataGenerator.ts

@@ -35,7 +36,6 @@ export class MetadataGenerator {

    this.checkForMethodSignatureDuplicates(controllers);
    this.checkForPathParamSignatureDuplicates(controllers);
-    this.circularDependencyResolvers.forEach(c => c(this.referenceTypeMap));


Handled in another place

nemnandor · 2023-10-19T09:59:53Z

packages/cli/src/metadataGeneration/metadataGenerator.ts

    }
-    this.referenceTypeMap[decodeURIComponent(referenceType.refName)] = referenceType;
+    this.referenceTypeMap[referenceType.refName] = referenceType;


decodeURIComponent() not interesting here, because typeResolver guarantees, that only OpenApi-accepted names can be generated.

nemnandor · 2023-10-19T10:08:05Z

packages/cli/src/metadataGeneration/metadataGenerator.ts

  }

  public GetReferenceType(refName: string) {
    return this.referenceTypeMap[refName];
  }

-  public OnFinish(callback: (referenceTypes: Tsoa.ReferenceTypeMap) => void) {
-    this.circularDependencyResolvers.push(callback);
+  public CheckModelUnicity(refName: string, positions: Array<{ fileName: string; pos: number }>) {


Type unicity check:

We want to check, that type A<B<...>,{ c: C}> is uniq or not. For this, we need to check if every A, B & C type identifier definitions are uniq or not. Uniq = A type can have more than one definition, but their definition positions array must be uniq. Position = fileName + pos. Not stored, so not interesting, which local machine the check is made.

A and A_B_ types are both renamed to A_B_ to use it in OpenApi. This is a second type of name conflicts.

nemnandor · 2023-10-19T10:12:43Z

tests/fixtures/testModel.ts

+    jsonCharacters: undefined; //type is not really interesting
+  };
+
+  jsDocTypeNames?: {


This types are different because of their jsdoc comments. Should generate different TSOA types for it (with, of course, different names).
Test checks situations which were handled previously as Partial_any_ to cover a more special situation

nemnandor · 2023-10-19T10:13:52Z

tests/fixtures/testModel.ts

+    }>;
+  };
+
+  jsdocMap?: {


Shows how to change jsdocs while changing the interface with mapped types

nemnandor · 2023-10-19T10:20:17Z

tests/fixtures/testModel.ts

+    synonym2: JsDoccedSynonym2;
+  };
+
+  duplicatedDefinitions?: {


identifiers with more than one definition

nemnandor · 2023-10-19T10:21:36Z